BC5 readiness: pairwise BC4↔BC5 + check-bc5-compat orchestrator + scheduled CI#308
Conversation
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
There was a problem hiding this comment.
Pull request overview
Adds the missing “BC4 ↔ BC5 additive-only invariant” layer to the live canary by introducing pairwise assertions, a snapshot comparison script, an end-to-end Makefile orchestrator, and a nightly GitHub Actions workflow to run the pipeline.
Changes:
- Extend the conformance test schema + live surface test fixture with
pairwiseAssertionsrules (superset arrays/keys, equality, and allowlisted deltas). - Add
scripts/compare-canary-runs.shto apply pairwise rules across two backend snapshot directories. - Add Makefile orchestration targets (
conformance-live,check-bc5-compat) and a scheduled workflow that runs the canary and uploads artifacts.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/compare-canary-runs.sh |
New pairwise snapshot comparator that enforces additive-only invariants between BC4 and BC5. |
Makefile |
Adds orchestration targets to run BC4 pass, BC5 pass, then pairwise compare. |
conformance/schema.json |
Adds schema support for pairwiseAssertions rule definitions. |
conformance/tests/live-my-surface.json |
Adds canonical pairwise rules for GetMyNotifications (memories array + top-level keys). |
.github/workflows/live-canary.yml |
Nightly + manual workflow to run make check-bc5-compat and upload snapshots as artifacts. |
CONTRIBUTING.md |
Documents pairwise comparison semantics, orchestrator usage, and scheduled CI behavior. |
COORDINATION.md |
Minor update reflecting that PR4 plan item is now landed here. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a663ad9301
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
5 issues found across 7 files
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
… rule Post-launch reconciliation for the pairwise BC4<->BC5 canary. Clears the 14 review threads on #308 and corrects the memories rule to match what BC5 master actually shipped. scripts/compare-canary-runs.sh: - P0 (codex/cubic): match the TS live runner's snapshot filename scheme exactly -- sanitize the test name with [^A-Za-z0-9_-]+ -> "_" (case preserved), not lowercase/hyphen/operation-first. The old form matched no files, so check-bc5-compat could report clean without comparing anything. - P1 (cubic): a declared pairwise test missing a snapshot on either backend is now a hard error (exit 2), not a silent skip-and-pass. - empty `paths` loop (copilot): guard ${#RULE_PATHS[@]} instead of "${RULE_PATHS[@]:-}" so an empty array can't run against the body root. - pairwiseEqual (cubic): compare via jq deep-equality so object key-order differences don't false-fail. - exit-code contract (copilot): snapshot-read jq failures remap to the documented exit 2 instead of leaking jq's status under set -e. conformance/schema.json: pairwiseAssertions[].paths gains minItems:1. Makefile: - rm -rf "$(LIVE_RECORD_DIR)" guarded against empty / "/" / "..". - conformance-live validates LIVE_RECORD_DIR/BASECAMP_BACKEND before the TS live pass (invoked from the recipe after the guards) rather than after it. - check-bc5-compat comment: "reports all violations", not "fails on first". - new check-compare-canary target, wired into `make check`. .github/workflows/live-canary.yml: default BASECAMP_HOST in the workflow expression so an unset optional secret can't clobber the Makefile default. CONTRIBUTING.md: align the "all violations" wording; reframe the memories example to the waiver pattern. memories canary rule (conformance/tests/live-my-surface.json): the prior "BC3 commit 64acf34 aliases memories[] to bubble_ups[]" claim is counterfactual -- that commit does not exist, and BC5 master shipped `json.memories []` while BC4 four still populates it. The pairwiseSupersetArray rule now states the intended invariant and is waived by a temporary pairwiseDeltaAllowed entry pointing at spec/api-gaps/memories-emptied-regression.md (added on the reconciliation branch); remove the waiver once BC3 #10947 (which carries `json.memories @bubble_ups`) merges. scripts/test-compare-canary-runs.sh: network-free regression coverage for the filename scheme (incl. proof the old form finds nothing), missing-snapshot hard-fail, memories-waiver scoping, pairwiseEqual key-order, and the empty-paths guard.
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The waiver `reason` strings read "BC4 four still populates it", which scans as a typo to anyone without the post-launch topology in hand (BC4 is the `four` branch). Reword to "BC4 (the four branch)" in the live-my-surface.json description + pairwiseDeltaAllowed reason and the CONTRIBUTING.md example. Addresses the Copilot re-review on PR #308.
…s branch The brief's SDK-absorption runbook read as if the pairwiseDeltaAllowed waiver and conformance/tests/live-my-surface.json were present here, but those canary files are on PR #308's branch (this branch is the registry record only). Reword to state #308 introduces the waiver and that it gets removed from live-my-surface.json once #308 and BC3 #10947 both land. Also reframe the "no Smithy change" bullet to "no structural change" — this PR does realign the memories doc comment (documentation, not a shape change).
COORDINATION.md said the pairwiseSupersetArray rule "is waived via pairwiseDeltaAllowed", which reads as if the canary surface exists on this branch — it does not (conformance/tests/live-my-surface.json is on PR #308). Reword to attribute the rule + waiver to #308's branch, matching the same fix already made in the memories-emptied-regression brief.
…s branch The brief's SDK-absorption runbook read as if the pairwiseDeltaAllowed waiver and conformance/tests/live-my-surface.json were present here, but those canary files are on PR #308's branch (this branch is the registry record only). Reword to state #308 introduces the waiver and that it gets removed from live-my-surface.json once #308 and BC3 #10947 both land. Also reframe the "no Smithy change" bullet to "no structural change" — this PR does realign the memories doc comment (documentation, not a shape change).
COORDINATION.md said the pairwiseSupersetArray rule "is waived via pairwiseDeltaAllowed", which reads as if the canary surface exists on this branch — it does not (conformance/tests/live-my-surface.json is on PR #308). Reword to attribute the rule + waiver to #308's branch, matching the same fix already made in the memories-emptied-regression brief.
bb85531 to
0d64a65
Compare
The bullet suggested pages[*] was for how-many-pages checks, but pairwiseSupersetArray compares the total item count across pages (absent page values contribute 0, non-array page values are invalid). Say so, so operators don't write rules against the old semantics.
schema.json requires a reason on pairwiseDeltaAllowed, but the compare script also runs standalone (check-bc5-compat, the scheduled workflow) with no schema-validation step in front of it — a reason-less waiver still flattened into ALLOW_PATHS and suppressed enforcement unaudited. The extraction now rejects waivers without a non-empty string reason (exit 2). Scenario R pins it; scenario P's assertion follows the combined waiver error message.
A LIVE_RECORD_DIR (or, theoretically, a mktemp result) beginning with '-' would be parsed as an rm option; end option parsing explicitly.
The compare + harness scripts guard for it with a clear error; the Makefile comment now says so up front for contributors running make check on stock macOS bash.
…waiver reason * Each snapshot's recorded .operation must match the test entry's operation before comparison — a stale or overwritten file with the right name exits 2 instead of being compared as this test's capture. Scenario S pins it. * conformance/schema.json aligns with the runtime waiver guard: the pairwiseDeltaAllowed conditional now sets minLength 1 on reason, so an empty-string reason fails schema validation the same way it fails the compare script. TS conformance suite re-run green (84 passed).
schema.json's minItems:1 on paths doesn't run in front of standalone invocations (check-bc5-compat, the scheduled workflow). An empty waiver paths array is functionally harmless (it waives nothing) but is always a fixture mistake — reject it with the same exit-2 operator error as the other waiver shape violations. Scenario T pins it.
… message * The rm -rf guard now refuses '..' only as a PATH SEGMENT (leading, interior, trailing, or the whole path) instead of any substring, so benign names like tmp/live-canary..pr308 are allowed while every traversal form still refuses. Patterns unit-verified for all six shapes. * The regression harness preflights jq with the same clear error as the compare script instead of failing mid-setup with a raw command-not-found. * snapshot_read_error's hint no longer blames only malformed JSON — an invalid jq path expression built from a rule's path entry lands in the same handler.
The aggregate is a list, not an object — the rule flags it as an invalid target rather than evaluating it. Say so and point rule authors at pages[N].body.X for keys checks on paginated endpoints.
The aggregate handling keyed off any '[*]' substring, broader than the documented pages[*].body.X form — an undocumented items[*].foo would stream through jq with undefined comparison semantics instead of surfacing the fixture mistake. Both the read and the supersetArray totals branch now see only validated leading-pages[*] paths; anything else exits 2 as an unsupported path. Scenario U pins it.
* CONTRIBUTING's prerequisites now list the shared tooling make check needs beyond per-SDK toolchains: jq, and bash >= 4.4 for the pairwise-canary scripts (macOS /bin/bash is 3.2 — brew install bash; the scripts fail fast with that exact hint). * Scenario U2 pins that a second '[*]' after a valid leading pages[*] is rejected as unsupported (the guard already covered it).
* An absolute LIVE_RECORD_DIR equal to the repo checkout (or any of its ancestors, e.g. LIVE_RECORD_DIR=$PWD or $HOME) passed the guards and rm -rf'd the working tree before the canary ran. A new prefix check refuses any path the checkout lives under; absolute paths elsewhere (/tmp/canary) and in-checkout subdirs still work. Patterns unit-verified for checkout, $HOME, /tmp, relative, and in-checkout-absolute shapes. * The harness's mktemp gets an explicit template — current macOS accepts a bare -d (empirically verified here), but older BSD variants insist on a template and it costs nothing.
… divergence bc3's BC5 API train shipped 2026-07-18..21 and settled the memories question: doc/api/sections/my_notifications.md (bc3 #11628) documents memories as an always-empty placeholder superseded by bubble_ups, and bc3 #10947 — the once-planned `json.memories @bubble_ups` alias — is closed unmerged. The pairwiseDeltaAllowed entry is therefore not a temporary waiver pending a fix; it is the permanent machine-readable record of an accepted BC4→BC5 subtractive delta. Update the GetMyNotifications test description and both rule reasons in live-my-surface.json, and align the canonical example + prose in CONTRIBUTING.md, replacing the 'drop the waiver once #10947 merges' guidance with the actual retire conditions: BC4 empties memories too, or BC5's documented contract changes.
is_allowed's early continue ran before the '[*]' syntax guard, so an unsupported path (items[*].foo, a double star) that also appeared in a pairwiseDeltaAllowed waiver exited clean instead of reporting the fixture error — a typo in a waived canary rule could permanently suppress enforcement without ever surfacing. Validate path syntax first, then apply the allowlist skip. Adds scenario U3 covering the waived-unsupported-path case. Flagged by Codex review.
The unknown-RULE_TYPE rejection lived in the dispatch case inside the per-path loop, which only runs for paths that survive the is_allowed skip. A misspelled type (pairwiseSupersetAray) whose paths were all waived by pairwiseDeltaAllowed therefore exited clean — an operator error silently suppressed, and the typo'd rule never enforced anything again. Hoist the type check to rule extraction, before any per-path processing; the case's catch-all arm stays as defense in depth. Adds scenario V covering the waived-misspelled-type case. Flagged by Codex review.
The star guard only matched the literal '[*]', so a bare jq-stream typo like items[] (or pages[0].body.items[]) reached read_value unwrapped: the stream made the length variables multi-line/empty, the -lt comparison errored falsy, and a real shrink violation (BC4 [1,2] vs BC5 []) exited 0 — a false-green on exactly the invariant the script enforces. Generalize the guard: brackets are only accepted as a leading pages[N] or pages[*] segment (regex-validated), everything else is rejected as a fixture mistake with exit 2, still ahead of the waiver skip. Adds scenario W reproducing the items[] false-green. Flagged by Codex review.
- extract validate_rule_path() and apply it to pairwiseDeltaAllowed entries too: a malformed bracket path appearing only in a waiver names a path no enforcing rule can ever target and previously sat unnoticed - require bracketed paths to enter the page body (pages[N].body...): page-level keys other than body are backend-specific metadata with no pairwise semantics, and a typo like pages[0].items read null on both snapshots and false-greened superset rules - validate_snapshot now type-checks page keys (status number, headers object, bodyText string, url string) so typed corruption fails loud instead of reading as nulls - check-bc5-compat rm guard: canonicalize an existing LIVE_RECORD_DIR (cd + pwd -P) before the checkout-ancestor check so non-canonical spellings can't slip past the string compare; a failed cd empties LRD, which the ancestor pattern then refuses Regression scenarios X (waiver-only malformed path), Y (bracket path skipping .body), Z (wrongly-typed page keys) added; 29/29 green.
A concrete pages[N] rule asserts the page exists on both backends. When either capture had fewer pages, read_value returned null and the rule passed clean — hiding a fixture typo (pages[1] against single-page captures) or a genuine page-count regression. Both demand operator attention: fail with exit 2 naming the short snapshot and its page count. Cross-page-count comparison remains the job of pages[*] aggregation. Regression scenarios AA (absent from both) and AB (one-sided page loss, error names the bc5 snapshot) added; 31/31 green.
…kets
A path with jq punctuation but no brackets ('items, .pages') slipped the
bracket blacklist and interpolated into the jq program as raw syntax.
Terminate the class: validate_rule_path now admits only the documented
grammar — dotted [A-Za-z0-9_-] key segments, optionally led by a
pages[N]/pages[*] indexer that must enter the page body, plus the ""
body-root sentinel — and rejects everything else as a fixture mistake.
Regression scenario AC (comma path) added; 32/32 green.
…snapshots
- validate_rule_path: remove '-' from the segment charset — '.foo-bar'
parses in jq as subtraction ('.foo - bar'), not a key lookup, so a
hyphen-keyed rule evaluated to garbage instead of reading the field.
No fixture path uses hyphens; keys requiring quoting are a future,
deliberate extension of to_jq_path. Regression scenario AD added
(33/33 green).
- live-canary.yml: upload only the decode/ result trees. Wire snapshots
carry raw response bodies and captured headers from a live account and
must not persist as downloadable workflow artifacts; decode results
carry only booleans, error strings, and dotted field paths.
The round-7 canonicalization rewrote LRD itself, so rm -rf on a symlinked LIVE_RECORD_DIR deleted the tree the link points to instead of the link — a behavior change with data-loss potential. Canonicalize into a separate guard variable for the checkout-ancestor check and keep the rm on the original (trailing-slash-stripped) path: a symlink is removed as a link, while a symlink pointing into the checkout is still refused by the guard (verified both by hand).
A pairwiseEqual failure printed the complete BC4 and BC5 JSON values into the comparison output, which the scheduled canary lands in retained, log-access-readable Actions logs — leaking live-account field content (names, URLs, notification text) on any drift. Report only shape summaries (type + keys/length) and point the operator at a local re-run for raw values. Regression scenario AE proves the violation still fires while the content stays out of the output; 34/34 green.
Completes 5270e94: the invalid-target branches of pairwiseSupersetArray and pairwiseSupersetKeys still embedded the full BC4/BC5 JSON values in violation output. Factor the shape summary into a shared shape_of() helper and use it at all three sites; no violation message carries live field content anymore (grep-verified zero BC4=$BC4_VAL sites).
Shorthand paths ("" or "foo.bar") normalize to pages[0].body — fine
for single-page captures, but against a multi-page snapshot they
silently checked only page 0 and ignored the rest, under-enforcing the
rule. Hard-error with a pointer to pages[N]/pages[*] syntax as soon as
either side's capture has more than one page. Regression scenario AF
added; 35/35 green.
The \| alternation in a plain grep pattern is a GNU basic-regex extension; BSD grep treats it literally. grep -E with an unescaped pipe is unambiguous everywhere.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b45d014575
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A symlink to / (or /.) passed the checkout-ancestor guard by string accident: the canonical root makes the pattern '//' + '*', which never matches an absolute pwd. No destructive path actually remained — rm operates on the original path, so a root symlink lost only the link and rm refuses trailing-dot operands — but the guard's invariant should be explicit, not an accident of rm semantics. Refuse a canonical '/' outright (verified by hand with a symlink to /).
Summary
Closes the BC5-readiness canary loop: PRs 1–3 built the per-backend live capture (TS) and cross-language decode (Ruby/Python/Go/Kotlin) infrastructure. PR 4 adds the pairwise BC4↔BC5 comparison that catches the additive-only invariant — and the orchestrator + scheduled CI that runs the whole pipeline end-to-end on a daily cron.
Stacked on #301 (PR 3 — cross-language wire-replay decoders). Once #301 merges, this PR retargets
main.Why pairwise
Per-backend schema validation alone can't detect a class of regression: with every new BC5 field marked optional, a case where BC4 emits
memories: [items]and BC5 emitsmemories: []passes each backend's schema independently — yet that's exactly the additive-only invariant the canary should catch. This is not hypothetical: BC5 ships precisely this divergence — now codified as documented contract (see below).The canonical canary rule lives on
GetMyNotifications. The intended invariant: BC5memories[]should remain a superset of BC4's. But BC5 shipsjson.memories []while BC4 (thefourbranch) still populates it — and bc3 has settled this as a permanent, documented contract divergence:doc/api/sections/my_notifications.md(bc3 #11628) codifiesmemoriesas an always-empty placeholder superseded bybubble_ups, and the once-planned alias (json.memories @bubble_ups, bc3 #10947) never shipped — #10947 is closed unmerged. So thepairwiseSupersetArray: ["memories"]rule stays as the encoded additive-only invariant, permanently waived by thepairwiseDeltaAllowed: ["memories"]entry — the machine-readable record of the accepted BC4→BC5 subtractive delta, tracked inspec/api-gaps/memories-emptied-regression.mdand indexed by the public compatibility report. Retire the waiver only if BC4 emptiesmemoriestoo (the superset rule then passes clean) or BC5's documented contract changes.What's in this PR
conformance/schema.json—pairwiseAssertions[]with four rule types:pairwiseSupersetArray,pairwiseSupersetKeys,pairwiseEqual,pairwiseDeltaAllowed(with requiredreasonso a future public compatibility report can index accepted divergences).conformance/tests/live-my-surface.json— thememoriesSupersetArray rule + a top-level SupersetKeys rule onGetMyNotifications.scripts/compare-canary-runs.sh— applies pairwise rules to a pair of snapshot directories. Exit 0 clean / 1 violation / 2 operator error.Makefile—make conformance-live(one backend's capture + 4 replays) andmake check-bc5-compat(BC4 pass + BC5 pass + pairwise). Both opt-in; not inmake check..github/workflows/live-canary.yml— nightly cron +workflow_dispatch, gated on repo secrets (no-ops with a log message when missing rather than failing the run). Uploads snapshots as a 14-day artifact so failures can be inspected post-hoc without rerunning.CONTRIBUTING.md— new "Pairwise BC4↔BC5 comparison", "Orchestrator", and "Scheduled CI" subsections under "Live canary".Path syntax (dotted identifiers on each snapshot)
""(empty string) addresses the body root.foo.bardefaults topages[0].body.foo.bar— the common single-page case.pages[N].body.Xaddresses a specific page.pages[*].body.Xaggregates across pages into a list (each page's value becomes one element; useful when you want a per-page count, not a sum of inner lengths).Test plan
jq empty conformance/schema.json— clean JSONjq empty conformance/tests/live-my-surface.json— clean JSONbash -n scripts/compare-canary-runs.sh— shell syntax cleanpairwiseSupersetArrayflags BC5 memories shorter than BC4pairwiseSupersetKeysflags BC5 missing a top-level key BC4 haspairwiseEqualflags value divergencepairwiseDeltaAllowedwaives a flagged pathpages[*].bodyaggregation collects per-page valuesmake -n check-bc5-compatresolves correctlyactionlint .github/workflows/live-canary.ymlcleanzizmor .github/workflows/live-canary.ymlcleanmake validate-api-gapscleanmake check-bucket-flat-paritycleanOperational requirement
Pairwise rules assume identical account state across the BC4 and BC5 runs. The same project list, same notifications, etc. Without that, naturally-drifting collections (
unreads, etc.) will false-fail rules. CONTRIBUTING.md calls this out explicitly; the canary needs a dedicated test account with stable, equivalent fixtures replicated to each backend before scheduled CI can pass meaningfully.Sequencing
After this lands, the BC5-readiness canary infrastructure is feature-complete (PRs 1–4). Remaining BC5 readiness work:
scripts/detect-api-gaps.sh+ weekly cron workflow (api-gap detection tooling; validation already shipped in PR 1).main).Summary by cubic
Adds pairwise BC4↔BC5 comparison with a
check-bc5-compatorchestrator, nightly CI, and a hardened compare script + Makefile backed by a regression harness. The canonicalmemoriesrule stays encoded and is permanently waived viapairwiseDeltaAllowedto reflect BC5’s documented always‑emptymemories; violation output redacts live values, path grammar is strictly validated, andpages[*]compares item totals.New Features
conformance/schema.json:pairwiseAssertions[]withpairwiseSupersetArray,pairwiseSupersetKeys,pairwiseEqual,pairwiseDeltaAllowed;pathsrequire at least one entry; waivers require non‑emptyreason.make conformance-liveandmake check-bc5-compat;.github/workflows/live-canary.yml(nightly + manual, gated on secrets, defaultsBASECAMP_HOSTtohttps://3.basecampapi.com, uploads decode‑only artifacts; excludeswire/).conformance/tests/live-my-surface.json:GetMyNotificationscarries top‑levelpairwiseSupersetKeysand a SupersetArray rule onmemories, permanently waived viapairwiseDeltaAllowed(BC5’s documented always‑emptymemories).make check-compare-canaryrunsscripts/test-compare-canary-runs.sh(bash ≥ 4.4,jqrequired; uses portablegrep -Eon BSD/macOS).Bug Fixes
operation; strict path grammar (only dotted segments or leadingpages[N]/pages[*]into.body; no hyphens or jq punctuation; rejects bare[]streams and non‑leading[*]); shorthand paths are forbidden once either capture paginates;pages[N]must exist on both;pages[*]arrays compare total item counts;pairwiseSupersetKeysrejects non‑object targets and does not acceptpages[*];pairwiseEqualuses deep equality and fully redacts live values in messages.pathsmust be non‑empty arrays of strings; waivers require non‑emptyreasonand non‑emptypaths; waiver paths validated like enforcing paths (waivers do not bypass path checks).LIVE_RECORD_DIR; safe cleanup with segment‑aware..checks, checkout‑ancestor refusal, full trailing‑slash strip, refusal when the canonicalized path is the filesystem root, andrm -rf --on the original path; GNU make 3.81‑safe env defaults; fast env preflight;check-compare-canarywired intomake check; CI artifacts upload decode results only and exclude rawwire/.Written for commit 881138e. Summary will update on new commits.